chore!: remove search flag from docs command#470
chore!: remove search flag from docs command#470lukegalbraithrussell wants to merge 3 commits intodocs-search-subcommandfrom
Conversation
cmd/docs/docs.go
Outdated
| "Invalid docs command. Did you mean to search?", | ||
| if cmd.Flags().Changed("search") { | ||
| return slackerror.New("docs_search_flag_removed").WithMessage( | ||
| "The --search flag has been removed.", |
There was a problem hiding this comment.
Claude really didn't like me doing this but I think it's kinda polite?
There was a problem hiding this comment.
🧪 praise: This is quite clever erroring!
👾 ramble: I might agree that it'd be better to error without obvious remediation for a small few reasons:
- The remediation isn't unusual of following "unknown flag" with "--help" IMHO
- This requires some extra amount of code to consider with future changes
- We might be quick to change the familiar flag behavior with the upcoming release
- Possible adding this back in later releases causes a confusing error pattern
🔬 thought: Of course this is personal preference and I leave preferred remediation to you! But these breaking change give us a good chance to start fresh-
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## docs-search-subcommand #470 +/- ##
==========================================================
- Coverage 71.26% 71.17% -0.09%
==========================================================
Files 222 222
Lines 18658 18631 -27
==========================================================
- Hits 13296 13261 -35
- Misses 4181 4187 +6
- Partials 1181 1183 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
changed the title
zimeg
added
code health
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@lukegalbraithrussell LGTM but I left a comment that suggest we can remove all references of the --search flag including remediation workarounds. No blocker!
Re: Merging to main or the base branch - I think either is alright! But I might've merged this to main foremost. Thanks for making this a separate PR! 🏁
cmd/docs/docs.go
Outdated
| "Invalid docs command. Did you mean to search?", | ||
| if cmd.Flags().Changed("search") { | ||
| return slackerror.New("docs_search_flag_removed").WithMessage( | ||
| "The --search flag has been removed.", |
There was a problem hiding this comment.
🧪 praise: This is quite clever erroring!
👾 ramble: I might agree that it'd be better to error without obvious remediation for a small few reasons:
- The remediation isn't unusual of following "unknown flag" with "--help" IMHO
- This requires some extra amount of code to consider with future changes
- We might be quick to change the familiar flag behavior with the upcoming release
- Possible adding this back in later releases causes a confusing error pattern
🔬 thought: Of course this is personal preference and I leave preferred remediation to you! But these breaking change give us a good chance to start fresh-
|
@zimeg appreciate the blessed moon reviews. i removed the custom error |
2 participants
Changelog
--searchflag is now pointless givensearchsubcommand.Separate PR for ease-of-reviewing. Can merge into search subcommand or main!! up to CLI team
Summary
(Please describe the goal of this pull request and mention any related issue numbers)
Requirements